Feat/dependent feature toggles #140
Conversation
client.go
Outdated
| if parent.Variants != nil && len(*parent.Variants) > 0 { | ||
| variantName := uc.getVariantWithoutMetrics(parent.Feature, WithVariantContext(context)).Name | ||
| if contains(*parent.Variants, variantName) { | ||
| continue |
There was a problem hiding this comment.
not sure how idiomatic continue is in golang but I'd rather negate the contains and early exit with return false
| @@ -345,6 +340,48 @@ func (uc *Client) isEnabled(feature string, options ...FeatureOption) api.Strate | |||
| } | |||
| } | |||
|
|
|||
| func (uc *Client) isParentDependencySatisfied(feature *api.Feature, context context.Context) bool { | |||
There was a problem hiding this comment.
func (uc *Client) isParentDependencySatisfied(feature *api.Feature, context context.Context) bool {
if feature == nil || feature.Dependencies == nil || len(*feature.Dependencies) == 0 {
return true
}
for _, parent := range *feature.Dependencies {
parentToggle := uc.repository.getToggle(parent.Feature)
isEnabled := uc.isEnabled(parent.Feature, WithContext(context)).Enabled
if parentToggle == nil ||
(parentToggle.Dependencies != nil && len(*parentToggle.Dependencies) > 0) ||
(parent.Enabled != nil && !isEnabled) {
return false
}
if parent.Enabled == nil {
variantName := uc.getVariantWithoutMetrics(parent.Feature, WithVariantContext(context)).Name
hasValidVariant := parent.Variants != nil && len(*parent.Variants) > 0 && contains(*parent.Variants, variantName)
if !hasValidVariant && !isEnabled {
return false
}
}
}
return true
}
There was a problem hiding this comment.
Unless we write an every helper, we can't return out here or we will exit the entire function without properly looping over all the dependencies.
api/feature.go
Outdated
| Dependencies *[]FeatureDependencies `json:"dependencies"` | ||
| } | ||
|
|
||
| type FeatureDependencies struct { |
There was a problem hiding this comment.
I called this just Dependency in Node SDK with an option to other dependencies. The property inside Feature is indicating what type of dependency it is. If we don't expose this type in the public API then I'm ok with any name.
There was a problem hiding this comment.
Dependency works fine
client.go
Outdated
| } | ||
|
|
||
| if f.Dependencies != nil && len(*f.Dependencies) > 0 { | ||
| parentEnabled := uc.isParentDependencySatisfied(f, *ctx) |
There was a problem hiding this comment.
isn't it more than parent enabled? the variable name may be misleading
| return handleFallback(opts, feature, ctx) | ||
| } | ||
|
|
||
| if f.Dependencies != nil && len(*f.Dependencies) > 0 { |
There was a problem hiding this comment.
I'm curious why you decided to do this check outside the isParentDependencySatisfied. I made the opposite decision but didn't have a strong opinion here
There was a problem hiding this comment.
To me it made more sense readability wise. If I don't have dependencies or the length of dependencies is 0 then I don't need to do check if isParentDependencySatisfied(f, *ctx). Moving it inside that function obfuscates the check that determines whether or not that function should actually be run. I don't have strong opinions about this either, but I found it easier to reason about this way because now the function is limited to doing one thing and one thing only.
Implements dependent feature toggles for the golang-sdk